feat(vscode): Implements readFile/writeFile for workspace.fs #6980

benoitf · 2020-01-28T08:29:53Z

What it does

Implements workspace.fsfor readFile and writeFile

How to test

readFile

        const sampleTxtAbsolutePath = context.asAbsolutePath('sample.txt');
	const fileToUse = vscode.Uri.file(sampleTxtAbsolutePath);
	let sampleTextEncoded = await vscode.workspace.fs.readFile(fileToUse);
	let sampleText = new util.TextDecoder('utf-8').decode(sampleTextEncoded);

writeFile

	const sampleTxtToWriteAbsolutePath = context.asAbsolutePath('sampleToWrite.txt');
	const fileToWrite = vscode.Uri.file(sampleTxtToWriteAbsolutePath);
	const contentToWrite = new util.TextEncoder().encode('Hello World');
	await vscode.workspace.fs.writeFile(fileToWrite, contentToWrite);

For other methods like stat, etc, need to revise Theia core stuff as Theia is not providing enough stats info for example

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

Change-Id: I1626547e72b8230bbc0a1832f682ebc18f2b22dc
Signed-off-by: Florent Benoit [email protected]

packages/plugin-ext/src/main/browser/file-system-main.ts

packages/plugin-ext/src/plugin/types-impl.ts

packages/plugin-ext/src/main/browser/file-system-main.ts

akosyakov · 2020-01-29T04:34:20Z

@benoitf sorry i could not attend the meeting yesterday, it should be fine to add required metadata to FileStat, @kittaakos ?

benoitf · 2020-01-29T07:30:56Z

@akosyakov about adding stuff about FileStat during the meeting we agreed to add stuff and deprecate other some methods (shouldn't be a problem)

A wider problem to me is that today we have FileSystem and ResourceProvider/ResourceResolver
new FileSystemProvider was hooked as a Resource (and then we have resources with read/write)

but then, to use createDir, getFileStat, copy/remove and all other methods of workspace.fs
https://github.com/microsoft/vscode/blob/1.39.2/src/vs/vscode.d.ts#L5809-L5874

we won't be able to use Resource API with that design , thoughts ?

kittaakos · 2020-01-29T07:41:50Z

required metadata

Did you mean mtime, ctime and the file type, such as directory, file, and symlink? Sure, why not. We retrieve the fs.Stats object in the FileSystemNode anyways.

theia/packages/filesystem/src/node/node-filesystem.ts

Line 446 in 3ad2b62

const stats = await fs.stat(FileUri.fsPath(uri));

benoitf · 2020-01-29T07:45:20Z

@kittaakos yes ctime, mtime, etc but also to have FileType instead of just isDirectory https://github.com/microsoft/vscode/blob/e6651b90f937e9874aeba717cb3724190e01a34f/src/vs/platform/files/common/files.ts#L206-L210

kittaakos · 2020-01-29T08:01:00Z

have FileType instead of just isDirectory

Nice. I would leave the isDirectory in place, you can depreciate it though so it does not break. Thoughts?

benoitf · 2020-01-29T08:02:03Z

@kittaakos yes add new stuff + keep it with deprecation

akosyakov · 2020-01-30T03:36:26Z

we won't be able to use Resource API with that design , thoughts ?

yes, I think in the plugin system to implement workspace.fs, we should use the file system directly then and also contributed file system providers. But we should still hook file system provider as resources that editors could read from them for custom schemes.

I wonder where else we should hook file system providers. Could they contribute somehow to the navigator or VS Code extension supposed to provide own tree views?

benoitf · 2020-02-03T10:24:36Z

PR updated

tsmaeder · 2020-02-03T11:30:38Z

packages/plugin-ext/src/plugin/in-plugin-filesystem-proxy.ts

+    async readFile(uri: UriComponents): Promise<Uint8Array> {
+        try {
+            const val = await this.proxy.$readFile(uri);
+            return new TextEncoder().encode(val);


I'm worried about this line: will it provide the exact byte contents of the file in every case? Reading the file as a string will use a certain text encoding. How can we be sure we're using the same encoding on this line?

@tsmaeder because it is using utf-8 ?

But you don't know that: resources are a point of extension. Say a resource reads a file in EBDIC and translates that correctly to a string. When you encode that using utf-8, you will not have the original byte contents in your byte array.

yes, it was like temporary issue (for non utf-8 resources) until being able to use filesystem from workspace.fs (and then hook FileStat etc in theia core)
For now we only have resources allowing the file access + custom FileSystemResourceProvider

tsmaeder · 2020-02-03T11:46:08Z

we won't be able to use Resource API with that design , thoughts ?

I think this is correct: Resource is the interface between the editor and content: FileSystem is the interface between storage and content.
Every file is a resource, but not every resource is a file. Also: files deal with bytes, resources with strings. Encoding needs to happen in between.

tsmaeder · 2020-02-03T11:47:05Z

Also, I was wondering why we need this now. Is there an issue this fixes or a particular VS Code extension it enables?

benoitf · 2020-02-03T12:25:10Z

Related VS Code extension: https://github.com/microsoft/vscode-extension-samples/blob/master/call-hierarchy-sample/src/extension.ts#L21

akosyakov · 2020-02-04T08:40:37Z

@tsmaeder Could you finish the review please?

benoitf · 2020-02-07T08:32:45Z

@tsmaeder ?

tsmaeder · 2020-02-07T08:52:14Z

I don't think we should merge this PR in the current form. The architectural problem that files are implemented in terms of resources leads to potential problems with encoding. I would agree to this workaround if there was a clear business reason to do it, but not to enable an example plugin (that's why I asked what it's for). Let's do this right! I might be swayed if there is a clear commitment to fix the issues soon.

benoitf · 2020-02-07T08:56:50Z

well, I would argue that there are other extensions using that
https://github.com/search?q=%22vscode.workspace.fs.readFile%22&type=Code

and from what I've seen, they're all using UTF-8.

but well, it's up to reviewers.

tsmaeder · 2020-02-07T09:23:59Z

I'm all for it, but let's do it right. Would it be so hard to fix the resource/file issue?

benoitf · 2020-02-07T09:26:28Z

I just wanted to proceed by steps

introduce readFile/writeFile
introduce stat/type API in theia core
Hook filesystemproviders into theia filesystem
Map fs API to theia filesystem

but of course we can omit step 1, just that it was easy to provide it like this without any changes yet in theia core.

tsmaeder · 2020-02-07T10:54:58Z

Why don't you open an issue for this so we can track it? Also, is there a way to test this in real life?

benoitf · 2020-02-07T11:06:05Z

what are you calling 'real life' ?
I will add issue

Also it has been discussed during theia dev meeting a couple of weeks ago

tsmaeder · 2020-02-07T11:26:57Z

Is there a plugin I can install and exercise the code?

benoitf · 2020-02-07T15:05:16Z

@tsmaeder so by looking at my github search link [1] it turns out that markdown language server is using the API [2]

And this extension is provided by default in Eclipse Theia.

[1] https://github.com/search?q=%22vscode.workspace.fs.readFile%22&type=Code
[2] https://github.com/microsoft/vscode/blob/master/extensions/markdown-language-features/src/features/workspaceSymbolProvider.ts#L89-L90

akosyakov

I think we can accept it, some small extension would be helpful to drop and verify? Or how you check it?

packages/plugin-ext/src/plugin/types-impl.ts

benoitf · 2020-02-10T12:32:52Z

How to check with callhierachy sample: (I haven't looked at how markdown LS is using readFile api)

start Theia with pre-build vsix on a gist from examples/browser directory

$ THEIA_PLUGINS=https://gist.github.com/benoitf/234f32b79ce621b7a6a2f6b2c13ddd24/raw/c796ba66654d3bdb86f70ff7d420e495e83314f2/call-hierarchy-sample-0.0.1.vsix  node src-gen/backend/main.js

Then create a dummy foo.txt file (to trigger actionEvent on the sample)
It will then create a new Editor based on a file read from the VS Code extension

Change-Id: I1626547e72b8230bbc0a1832f682ebc18f2b22dc Signed-off-by: Florent Benoit <[email protected]>

akosyakov

it works nicely for me

@tsmaeder could we merge it for now?

akosyakov · 2020-02-11T10:28:33Z

introduce readFile/writeFile
introduce stat/type API in theia core
Hook filesystemproviders into theia filesystem
Map fs API to theia filesystem

Regarding steps It is a bit unclear to me why do we need to hook plugin fs providers in core filesystem. Sorry i did not research how these providers should be used. But if they are need only for VS Code compatibility I think we can leave core filesystem as is and build something on top like a facade to core filesystem and plugin fs providers.

tsmaeder · 2020-02-12T10:15:58Z

@benoitf When I follow your testing instructions with with the call-hierarchy sample, a breakpoint in languages-main.ts#readFile is not hit.

tsmaeder · 2020-02-12T10:29:21Z

@benoitf When I follow your testing instructions with with the call-hierarchy sample, a breakpoint in languages-main.ts#readFile is not hit.

Stupid old me is using the "fixed" version you provided for the call hieararchy PR 🤦‍♂

tsmaeder · 2020-02-12T10:48:40Z

Hmh...still can't hit that breakpoint. I guess you have @akosyakov's approval, so you don't need mine.

benoitf · 2020-02-13T13:53:48Z

ok, I will merge today if no objections

kittaakos reviewed Jan 28, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/file-system-main.ts Outdated Show resolved Hide resolved

benoitf force-pushed the workspace-fs branch from 8a3f1ee to d0b7536 Compare January 28, 2020 09:09

benoitf added plug-in system issues related to the plug-in system vscode issues related to VSCode compatibility labels Jan 28, 2020

akosyakov requested review from azatsarynnyy, amiramw, vince-fugnitto and tsmaeder January 29, 2020 04:22

akosyakov reviewed Jan 29, 2020

View reviewed changes

packages/plugin-ext/src/plugin/types-impl.ts Outdated Show resolved Hide resolved

akosyakov reviewed Jan 29, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/file-system-main.ts Outdated Show resolved Hide resolved

akosyakov reviewed Jan 29, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/file-system-main.ts Outdated Show resolved Hide resolved

akosyakov reviewed Jan 29, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/file-system-main.ts Outdated Show resolved Hide resolved

akosyakov reviewed Jan 29, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/file-system-main.ts Outdated Show resolved Hide resolved

akosyakov added the filesystem issues related to the filesystem label Jan 29, 2020

akosyakov mentioned this pull request Jan 30, 2020

remove dependencies to plugin ext host code from other processes #7015

Open

benoitf force-pushed the workspace-fs branch 2 times, most recently from a77bc49 to b45f8c9 Compare February 3, 2020 09:56

tsmaeder reviewed Feb 3, 2020

View reviewed changes

akosyakov reviewed Feb 10, 2020

View reviewed changes

packages/plugin-ext/src/plugin/types-impl.ts Outdated Show resolved Hide resolved

feat(vscode): Implements readFile/writeFile for workspace.fs

cc5de52

Change-Id: I1626547e72b8230bbc0a1832f682ebc18f2b22dc Signed-off-by: Florent Benoit <[email protected]>

benoitf force-pushed the workspace-fs branch from b45f8c9 to cc5de52 Compare February 10, 2020 12:33

akosyakov approved these changes Feb 11, 2020

View reviewed changes

benoitf merged commit 4d4f6c2 into master Feb 14, 2020

benoitf deleted the workspace-fs branch February 14, 2020 08:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vscode): Implements readFile/writeFile for workspace.fs #6980

feat(vscode): Implements readFile/writeFile for workspace.fs #6980

benoitf commented Jan 28, 2020

akosyakov commented Jan 29, 2020

benoitf commented Jan 29, 2020

kittaakos commented Jan 29, 2020

benoitf commented Jan 29, 2020

kittaakos commented Jan 29, 2020

benoitf commented Jan 29, 2020

akosyakov commented Jan 30, 2020

benoitf commented Feb 3, 2020

tsmaeder Feb 3, 2020

benoitf Feb 3, 2020

tsmaeder Feb 3, 2020

benoitf Feb 3, 2020

tsmaeder commented Feb 3, 2020

tsmaeder commented Feb 3, 2020

benoitf commented Feb 3, 2020

akosyakov commented Feb 4, 2020

benoitf commented Feb 7, 2020

tsmaeder commented Feb 7, 2020

benoitf commented Feb 7, 2020

tsmaeder commented Feb 7, 2020

benoitf commented Feb 7, 2020

tsmaeder commented Feb 7, 2020

benoitf commented Feb 7, 2020

tsmaeder commented Feb 7, 2020

benoitf commented Feb 7, 2020

akosyakov left a comment

benoitf commented Feb 10, 2020

akosyakov left a comment

akosyakov commented Feb 11, 2020

tsmaeder commented Feb 12, 2020

tsmaeder commented Feb 12, 2020

tsmaeder commented Feb 12, 2020

benoitf commented Feb 13, 2020

feat(vscode): Implements readFile/writeFile for workspace.fs #6980

feat(vscode): Implements readFile/writeFile for workspace.fs #6980

Conversation

benoitf commented Jan 28, 2020

What it does

How to test

Review checklist

Reminder for reviewers

akosyakov commented Jan 29, 2020

benoitf commented Jan 29, 2020

kittaakos commented Jan 29, 2020

benoitf commented Jan 29, 2020

kittaakos commented Jan 29, 2020

benoitf commented Jan 29, 2020

akosyakov commented Jan 30, 2020

benoitf commented Feb 3, 2020

tsmaeder Feb 3, 2020

Choose a reason for hiding this comment

benoitf Feb 3, 2020

Choose a reason for hiding this comment

tsmaeder Feb 3, 2020

Choose a reason for hiding this comment

benoitf Feb 3, 2020

Choose a reason for hiding this comment

tsmaeder commented Feb 3, 2020

tsmaeder commented Feb 3, 2020

benoitf commented Feb 3, 2020

akosyakov commented Feb 4, 2020

benoitf commented Feb 7, 2020

tsmaeder commented Feb 7, 2020

benoitf commented Feb 7, 2020

tsmaeder commented Feb 7, 2020

benoitf commented Feb 7, 2020

tsmaeder commented Feb 7, 2020

benoitf commented Feb 7, 2020

tsmaeder commented Feb 7, 2020

benoitf commented Feb 7, 2020

akosyakov left a comment

Choose a reason for hiding this comment

benoitf commented Feb 10, 2020

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Feb 11, 2020

tsmaeder commented Feb 12, 2020

tsmaeder commented Feb 12, 2020

tsmaeder commented Feb 12, 2020

benoitf commented Feb 13, 2020